Skip to content

[sandbox audit] Keep secret values out of argv in the CLI and fix the env/secrets docs - #4835

Draft
Wauplin wants to merge 1 commit into
security/pin-and-verify-binaryfrom
security/cli-secrets-hygiene
Draft

[sandbox audit] Keep secret values out of argv in the CLI and fix the env/secrets docs#4835
Wauplin wants to merge 1 commit into
security/pin-and-verify-binaryfrom
security/cli-secrets-hygiene

Conversation

@Wauplin

@Wauplin Wauplin commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

[sandbox audit] — PR 9 of 9 in this repo's stack; merge in order.
Previous: #4837
Review only the commits this PR adds on top of its base; bases collapse to main as the stack lands.

Why

Two related problems with how the CLI handles secret material.

Secret values in argv. --secrets KEY=value and --token <value> both take the
value as a flag argument, so it ends up in argv. That is not a private channel: the
invocation is appended verbatim to ~/.bash_history / ~/.zsh_history, and on Linux
/proc/<pid>/cmdline is readable by every other process running as the same user (and by
root) for as long as the command runs. The CLI already had argv-free paths — bare
--secrets HF_TOKEN resolves the value from the calling environment, --secrets-file
reads it from disk, hf auth login / HF_TOKEN cover the token — but nothing pointed at
them, and the --secrets help text listed KEY=value first.

This is local hygiene, not a remote boundary: exploiting it needs access to the machine or
the history file, at which point the stored token in ~/.cache/huggingface is usually
available anyway. It is worth fixing because it is cheap and because the bare-name form
already existed.

The env/secrets docs were wrong. The sandbox section of guides/cli.md said to use
"-e / --secrets for environment variables". The real bindings are -e/--env and
-s/--secrets, and they are different channels with different storage: a user who
followed that line would put a secret through the unencrypted env channel, which for a
dedicated job means it lands in the job metadata rather than the encrypted secrets store.

What changed

src/huggingface_hub/cli/_cli_utils.py

  • SecretsOpt gets an option callback that warns on stderr, once per invocation
    (not per pair), when any --secrets value contains =. It names the two safer forms.
  • TokenOpt gets the same treatment for --token <value>, pointing at hf auth login
    and HF_TOKEN. hf auth login --token is exempt — that command is the recommended
    alternative, so warning there would contradict the advice.
  • --secrets-file - / --env-file - now read KEY=value lines from stdin. This is the
    only path where a value touches neither argv nor the disk.
  • Reading an env/secrets file warns if it is group- or world-readable (mode & 0o077),
    suggesting chmod 600. POSIX only: Windows reports a synthetic mode, so the check
    would fire unconditionally there.
  • --secrets help text now leads with the bare-name form.

src/huggingface_hub/cli/sandbox.py

  • The --pool + --secrets rejection now explains the trade-off instead of reading like
    a downgrade. A pooled sandbox has no encrypted-secrets channel, but its env is
    delivered to the host at creation and is not stored in the job metadata — better than
    a dedicated job's env, worse than a dedicated job's encrypted secrets. The message says
    exactly that.

docs/source/en/guides/cli.md

  • Fixed the -e / --secrets mix-up.
  • New "Pass environment variables and secrets to a sandbox" section with a table of the
    two channels and what each one means for a dedicated vs. a pooled sandbox.
  • The jobs section now leads with -s MY_SECRET (read from the environment) instead of
    -s MY_SECRET=psswrd, documents the stdin form, and explains the exposure.

docs/source/en/package_reference/cli.md is regenerated from the changed help strings
(python utils/generate_cli_reference.py --update) — python-quality checks it, so it
had to move with this change. Purely mechanical.

Deliberately not done: scrubbing argv

Overwriting the process title at startup would hide the value from ps and
/proc/<pid>/cmdline. In Python that needs setproctitle (a third-party dependency), it
does nothing about the shell history — which is the bigger half of the exposure — and by
the time the CLI runs, anything watching /proc may already have read the original
argv. The warning plus the bare-name / file / stdin paths cover the real exposure at a
fraction of the cost. Recording the decision here so it does not get re-litigated.

Behaviour changes

  • New warnings on stderr for --secrets KEY=value, --token <value>, and loose file
    permissions. Nothing becomes an error: a hard failure on --secrets KEY=value would
    break existing automation, which is not a proportionate response here.
  • The warnings honour the CLI's existing quiet mode (-q / --format quiet) — no new
    environment variable or flag was added. One pre-existing gap: pass-through commands
    (hf jobs run, hf sandbox exec, hf sandbox spawn, hf jobs uv run) forward
    unknown flags to the user's command, so the global formatting flags never reach them and
    the warning cannot be silenced there. Unchanged by this PR, worth knowing.
  • --secrets-file - / --env-file - were previously interpreted as a file literally
    named -. Anyone relying on that (nobody, plausibly) is affected.
  • hf sandbox create --pool <id> --secrets X=Y still errors, with the new wording.
  • The --secrets warning fires during option parsing, so on the --pool + --secrets
    path it is printed just before the error.

Validation

$ ruff check src/huggingface_hub/cli/ docs
All checks passed!

$ ruff format --check src/huggingface_hub/cli/
36 files already formatted

$ ty check src
... 4 diagnostics, all pre-existing (cli/_framework.py Click stubs, utils/_fixes.py)

$ pytest tests/test_cli.py tests/test_cli_framework.py tests/test_cli_output.py \
      tests/test_cli_errors.py tests/test_sandbox.py -q -k "cli or secret or env or Secret or Env"
3 failed, 451 passed, 26 deselected, 1 warning in 129.42s

The three failures are pre-existing on the base branch (verified by stashing this diff and
re-running): TestRepoListCommand::test_repo_list trips over a stale local hf_xet
(cannot import name 'XetSession' from 'hf_xet') and the two
TestSkillsMarketplaceCLI cases need network access I don't have. The plain
pytest tests/ -k "cli or secret or env" selection pulls in tests/test_buckets_cli.py,
which hangs against the staging Hub in my environment; the file list above is the same
selection minus those network suites.

New tests, in tests/test_cli.py::TestSecretHygiene (12, all passing):

$ pytest tests/test_cli.py -q -k TestSecretHygiene
12 passed
  • warning fires for --secrets KEY=value and for --token <value>
  • fires exactly once for three inline pairs
  • does not fire for bare --secrets KEY (and the value is still resolved from the
    environment), for --secrets-file, for --env only, or with no flag at all
  • does not fire for hf auth login --token
  • -q suppresses it entirely
  • --secrets-file - parses KEY=value from stdin
  • the permission warning fires on a 0644 secrets file and a 0640 env file, not on 0600
  • --pool + --secrets still errors, with the new explanation

🤖 Generated with Claude Code


Note

Low Risk
Changes are mostly warnings and documentation; existing --secrets KEY=value and --token flows still work, with new stderr output that scripts may need to handle unless -q is used.

Overview
Improves local secret hygiene for the hf CLI: inline --secrets KEY=value and --token <value> now emit stderr warnings (once per invocation, suppressed with -q) pointing users to env lookup, files, stdin, or hf auth login / HF_TOKEN. --env-file - and --secrets-file - read from stdin; env/secrets files on POSIX warn when group/world-readable. Shared option help text and regenerated package_reference/cli.md match the new guidance.

Docs fix the sandbox mix-up of -e vs --secrets, add a jobs/sandbox section on the env vs secrets channels (including pooled sandboxes: no encrypted secrets), and clarify pooled --pool + --secrets rejection with a clearer error message.

Reviewed by Cursor Bugbot for commit ce05550. Bugbot is set up for automated code reviews on this repo. Configure here.

@bot-ci-comment

bot-ci-comment Bot commented Sep 8, 2026

Copy link
Copy Markdown

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

`--secrets KEY=value` and `--token <value>` put secret material into
argv, where it is written to the shell history file and is readable
from `/proc/<pid>/cmdline` by any process running as the same user.
The CLI already had argv-free alternatives -- bare `--secrets KEY`
(resolved from the calling environment), `--secrets-file`,
`hf auth login`, `HF_TOKEN` -- but nothing steered users toward them.

Separately, the sandbox section of the CLI guide paired `-e` with the
secrets channel, while `-e` is `--env` and `-s` is `--secrets`. That
is not a cosmetic slip: a user following it puts a secret in the
unencrypted env channel, which for a dedicated job is stored in the
job metadata instead of the encrypted secrets store.

- warn on stderr, once per invocation, when `--secrets` carries an
  inline value or `--token` is given a value, pointing at the safer
  forms. `hf auth login --token` is exempt: it is the flow the
  warning itself recommends.
- warn when an env/secrets file is group- or world-readable
- support `--secrets-file -` (and `--env-file -`) to read `KEY=value`
  lines from stdin, so a value need touch neither argv nor the disk
- promote the bare-name form in the `--secrets` help text
- fix the `-e` / `--secrets` mix-up in `guides/cli.md` and document
  both channels and their storage properties in a table
- explain *why* a pooled sandbox rejects `--secrets` rather than only
  telling the user to switch to `--env`

These stay warnings on purpose: failing on `--secrets KEY=value`
would break existing automation. They are silenced by the CLI's
existing quiet mode (`-q` / `--format quiet`), so no new knob was
introduced.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@Wauplin
Wauplin force-pushed the security/cli-secrets-hygiene branch from a609837 to ce05550 Compare September 8, 2026 15:14
@Wauplin
Wauplin changed the base branch from security/sandbox-docs-accuracy to security/pin-and-verify-binary September 8, 2026 15:14
@Wauplin Wauplin changed the title Keep secret values out of argv in the CLI and fix the env/secrets docs [sandbox audit] Keep secret values out of argv in the CLI and fix the env/secrets docs Sep 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant